Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ROCm 6.0.1] Adaptation for HIPRTC changes in 6.0.1 #2641

Merged
merged 5 commits into from
Dec 28, 2023
Merged

[ROCm 6.0.1] Adaptation for HIPRTC changes in 6.0.1 #2641

merged 5 commits into from
Dec 28, 2023

Conversation

atamazov
Copy link
Contributor

@atamazov atamazov commented Dec 27, 2023 •
edited
Loading

  • 7e88449 regression: do not use file system symbolic/hard links (cherry-picked from #2425)
  • c4a091f Patch necessary to make FP8 convolution compile with hiprtc (cherry-picked from #2584)
  • 2c9fe96 Bump MIOpen version to 3.0.1 and update CI docker (cherry-picked from #2519 and EDITED)
    • Differences from #2519:
      • rocm_setup_version(VERSION 3.1.0) -> rocm_setup_version(VERSION 3.0.1)
      • Changes of requirements.txt REVERTED
  • c3de5e4 [ROCm 6.0.1][hipRTC] Fix build failures. [quality] Reorg standard includes in HIP sources. (cherry-picked from #2637 and EDITED)
    • Differences from #2637:
      • Almost all changes of test/gtest/CMakeLists.txt reverted, except:
        • extending timeout for gtest discovery
        • logging more testing parameters onto console
  • 4f695d9 Automatically activate the new HIPRTC PCH adaptations starting from the 6.0.24000. Fix some build errors.
    • ℹ️ You can use -DMIOPEN_OVERRIDE_HIP_VERSION_PATCH=XXXX to control this behaviour.
      • For example, if some future ROCm RC, say, 2.0.241111, contains old HIPRTC, then you may need to disable the new HIPRTC adaptations. This can be done by -DMIOPEN_OVERRIDE_HIP_VERSION_PATCH=23999 when building MIOpen.
      • Similarly, if you want to test MIOpen with some, for example. ROCm RC 6.0.23525, and that ROCm needs the new HIPRTC adaptations, then you can use -DMIOPEN_OVERRIDE_HIP_VERSION_PATCH=24000.

⚠️ If this PR works fine, then we'll have to cherry-pick commit 4f695d9 into develop.

  • That would maintain compatibility of future MIOpen releases with ROCm 6.0.1.
  • Please expect merge conflicts (the trivial ones, though) during cherry-picking because this commit is partially based on #2465 which has been already merged into develop.

⚠️ PLEASE DO NOT SQUASH THIS WHEN MERGING!

🌀 Testing

  • Navi21 (gfx1030/36), Ubuntu 20.04.5 LTS
  • Docker image: rocm/miopen:hiprtc with dependencies installed from requirements.txt from this PR.
  • -DMIOPEN_TEST_ALL=Off -DBUILD_DEV=On (Smoke testing)
  • -DMIOPEN_OVERRIDE_HIP_VERSION_PATCH=2400
  • Tested builds, compilation paths & datatypes:
    • FLOAT, offline compiler, release build
    • FLOAT, online compiler, DEBUG build
    • HALF, online compiler, release build
    • BFLOAT16, online compiler, release build
    • INT8, online compiler, release build

🟢 ALL PASSED

[Attribution] @junliume @JehandadKhan

apwojcik and others added 4 commits December 27, 2023 02:41
@apwojcik @atamazov
regression: do not use file system symbolic/hard links (ROCm#2425)
7e88449 
Co-authored-by: Artur Wojcik <artur.wojcik@amd.com>
Co-authored-by: JD <jahandad@gmail.com>
Co-authored-by: Jun Liu <Liu.Jun@amd.com>
(cherry picked from commit 31e8376)
@umangyadav @atamazov
Patch necessary to make FP8 convolution compile with hiprtc (ROCm#2584)
c4a091f 
(cherry picked from commit 7ae1553)

# RESOLVED Conflicts:
#	src/kernels/hip_float8.hpp
@atamazov
Bump MIOpen version to 3.0.1 and update CI docker (partial cherry-pic…
2c9fe96 
…k of ROCm#2519)

Differences from ROCm#2519:
- rocm_setup_version(VERSION 3.1.0) -> rocm_setup_version(VERSION 3.0.1)
- Changes of requirements.txt REVERTED

(cherry picked from commit 7da72bc and EDITED)

# RESOLVED Conflicts:
#	Dockerfile
#	dev-requirements.txt
#	requirements.txt -- REVERTED
@atamazov
[ROCm 6.0.1][hipRTC] Fix build failures. [quality] Reorg standard inc…
c3de5e4 
…ludes in HIP sources. (partial cherry-pick of ROCm#2637)

Differences from ROCm#2637:
- Almost all changes of test/gtest/CMakeLists.txt reverted, except extending timeout for gtest discovery and logging more testing parameters onto console.

(cherry picked from commit 3cc32a7 and EDITED)

# RESOLVED Conflicts:
#	test/gtest/CMakeLists.txt - EDITED
@atamazov
Copy link
Contributor Author

@junliume I see issues with some smoke_solver_ConvHipImplicitGemm* tests and with LayerNormTestSet. Investigating...

@atamazov
Automatically activate the new HIPRTC PCH workarounds starting from t…
4f695d9 
…he 6.0.24000 version. Fix some build errors (ROCm#2465 + more)
@atamazov atamazov marked this pull request as ready for review December 27, 2023 20:59
@atamazov
Copy link
Contributor Author

@junliume This is ready to go.

@junliume junliume merged commit a55aaa0 into ROCm:release/rocm-rel-6.0-staging Dec 28, 2023
1 check passed
@atamazov
Copy link
Contributor Author

@junliume Excuse me, but I do not understand why this branch has been squashed on merge.

@atamazov atamazov mentioned this pull request Dec 28, 2023
Merged
@junliume
Copy link
Collaborator

@junliume Excuse me, but I do not understand why this branch has been squashed on merge.

Ops, used default setting. The Cherry Pick history is still visible from the squashed commit message though. The staging has already started, I will see if there is a good way to "unsquash" the commits.

@atamazov
Copy link
Contributor Author

@junliume No problem for me. I cherry-picked #2644 from my development branch. Let's hope the maintainers of the release branch won't have any problems either.

atamazov
atamazov commented Jan 2, 2024
View reviewed changes
src/kernels/miopen_limits.hpp
Comment on lines +28 to +30
#ifndef WORKAROUND_DO_NOT_USE_CUSTOM_LIMITS
#define WORKAROUND_DO_NOT_USE_CUSTOM_LIMITS 0
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@junliume Another instance of the quality issue. Better to fix, but not mandatory.

@atamazov
Copy link
Contributor Author

atamazov commented Jan 3, 2024

@junliume Does this work correctly with ROCm 6.0.1 RC? If everything is fine in the release staging branch, then let's merge #2644.

@junliume
Copy link
Collaborator

junliume commented Jan 4, 2024

@junliume Does this work correctly with ROCm 6.0.1 RC? If everything is fine in the release staging branch, then let's merge #2644.

Still waiting for the staging results as of 01/03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAHEK7 CAHEK7 CAHEK7 approved these changes

@junliume junliume junliume approved these changes

Assignees
No one assigned
Labels
enhancement urgency_unknown value_high
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@atamazov @junliume @CAHEK7 @apwojcik @umangyadav